Feat incentive offers #258
Conversation
|
@github-actions[bot] is attempting to deploy a commit to the bunty's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request involve updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. In the meantime, please ensure that your changes align with our CONTRIBUTING.md. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
frontend/src/App.jsx (2)
12-12: Consider user experience for returning visitors.The
showModalstate is correctly implemented. However, setting it totrueby default means the offer will show every time the app loads, which might be intrusive for returning users.Consider using local storage or cookies to track if the user has seen the offer recently, and only show it to new visitors or after a certain time has passed.
Example implementation:
const [showModal, setShowModal] = useState(() => { const lastShown = localStorage.getItem('lastOfferShown'); const showAgain = !lastShown || (Date.now() - parseInt(lastShown)) > (24 * 60 * 60 * 1000); // 24 hours if (showAgain) { localStorage.setItem('lastOfferShown', Date.now().toString()); } return showAgain; });
16-16: LGTM: Offers component is correctly implemented.The Offers component is properly integrated into the App structure with correct props for visibility control and closing functionality. This aligns well with the PR objective of introducing an offer modal to enhance user engagement.
Consider memoizing the
onClosefunction to optimize performance, especially if the App component re-renders frequently:const onCloseModal = React.useCallback(() => setShowModal(false), []); // Then in the JSX: <Offers isVisible={showModal} onClose={onCloseModal} />frontend/src/components/Shared/Offers.jsx (1)
28-83: LGTM: Modal structure and styling are well-implemented. Consider improving accessibility.The modal structure is well-organized, and the use of Tailwind CSS classes provides consistent styling. The two-column layout with an image and text content is visually appealing. Good job on implementing hover effects for buttons to enhance user interaction.
To improve accessibility, consider the following enhancements:
- Add
aria-labelto the close button:<button className="absolute top-2 right-2 text-black text-xl" onClick={() => onClose()} + aria-label="Close offer modal" > <RxCross1 color='black' /> </button>
- Add
role="dialog"andaria-modal="true"to the main modal div:-<div className='fixed inset-0 bg-white bg-opacity-25 backdrop-blur-lg flex justify-center items-center z-30'> +<div className='fixed inset-0 bg-white bg-opacity-25 backdrop-blur-lg flex justify-center items-center z-30' role="dialog" aria-modal="true">
- Consider trapping focus within the modal when it's open to improve keyboard navigation.
These changes will make the modal more accessible to users relying on screen readers or keyboard navigation.
frontend/src/components/Shared/Navbar.jsx (1)
Line range hint
46-53: Approve logout functionality changes with a minor suggestion.The updates to the
handleLogoutfunction are well-implemented. They align with a token-based authentication system and provide a more comprehensive logout process. This contributes to a better user experience, which aligns with the PR objectives.Consider adding a success message or toast notification to inform the user that they have been successfully logged out. This could further enhance the user experience.
Example implementation:
const handleLogout = () => { Cookies.remove("authToken"); setToken(null); setIsModalOpen(false); setIsMenuOpen(false); // Add a success message here // toast.success("You have been successfully logged out"); navigate("/login"); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- README.md (6 hunks)
- frontend/src/App.jsx (2 hunks)
- frontend/src/components/Shared/Navbar.jsx (1 hunks)
- frontend/src/components/Shared/Offers.jsx (1 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md
164-164: Column: 1
Hard tabs(MD010, no-hard-tabs)
165-165: Column: 1
Hard tabs(MD010, no-hard-tabs)
208-208: Column: 1
Hard tabs(MD010, no-hard-tabs)
209-209: Column: 1
Hard tabs(MD010, no-hard-tabs)
252-252: Column: 1
Hard tabs(MD010, no-hard-tabs)
253-253: Column: 1
Hard tabs(MD010, no-hard-tabs)
296-296: Column: 1
Hard tabs(MD010, no-hard-tabs)
297-297: Column: 1
Hard tabs(MD010, no-hard-tabs)
🔇 Additional comments (13)
frontend/src/App.jsx (3)
3-4: LGTM: New imports are correctly added.The new imports for React, useState, and the Offers component are correctly placed and necessary for the implemented changes.
Also applies to: 10-10
25-25: LGTM: Export statement moved for better readability.Moving the export statement to a separate line is a good practice that improves code clarity without affecting functionality.
Line range hint
1-25: Summary: Implementation aligns well with PR objectives.The changes in this file successfully introduce the offer modal, aligning with the PR objective of enhancing user engagement through incentives and special offers. The modal's visibility is controlled by state, allowing for dynamic display.
To further improve on the objectives:
- Consider implementing a mechanism to show offers less frequently to returning users, as suggested earlier.
- Think about how this modal could be used to display different types of offers (e.g., "Product of the Day", "Deal of the Day") to align with the objective of implementing regular discounts and promotions.
- Consider adding a way for users to subscribe to offers directly from this modal, addressing the email subscription objective.
Overall, this is a solid foundation for the new feature. Great work!
frontend/src/components/Shared/Offers.jsx (4)
1-6: LGTM: Imports and component declaration are well-structured.The imports are appropriate for the component's functionality, and the component declaration follows React best practices. Good job on using named imports and declaring the component with clear prop names.
20-26: LGTM: Conditional rendering and navigation handling are well-implemented.The conditional rendering to return null when
!isVisibleis a good practice for performance. ThehandleTakeMeTherefunction correctly closes the modal before navigation, which helps prevent potential issues with modal state.
87-87: LGTM: Component export is correct.The default export of the Offers component follows common React practices and allows for easy import in other parts of the application.
1-87: Overall assessment: Well-implemented component meeting PR objectives.The Offers component successfully implements the requirements outlined in the PR objectives. It provides an appealing modal for displaying promotional offers, which can help improve customer retention and drive traffic to the website. The component is well-structured, uses React hooks effectively, and follows good coding practices.
Key strengths:
- Responsive design with a visually appealing two-column layout.
- Proper management of modal visibility and body scroll locking.
- Clear call-to-action buttons with hover effects.
- Integration with react-router for seamless navigation.
Areas for improvement:
- Enhance accessibility as suggested in previous comments.
- Consider using CSS classes for managing body overflow instead of direct DOM manipulation.
Overall, this component is a valuable addition to the application and aligns well with the goals of improving customer engagement and retention.
frontend/src/components/Shared/Navbar.jsx (3)
Line range hint
1-238: Summary of Navbar component changesOverall, the changes to the Navbar component align well with the PR objectives and contribute to improving the user experience. Key improvements include:
- Enhanced logout functionality with proper token management.
- Improved text formatting for consistency.
- Potential adjustment to the navbar's z-index for better layout management.
These changes support the goal of improving customer retention by providing a more polished and user-friendly interface. The logout process is now more secure and comprehensive, which indirectly contributes to better user engagement.
To further align with the PR objectives, consider the following suggestions:
- Implement the success message for logout as suggested earlier.
- Ensure consistent casing across all buttons in the application.
- Verify the z-index change doesn't negatively impact the navbar's visibility or functionality.
These enhancements will further contribute to a seamless user experience, potentially increasing user engagement with the café's menu and special offers.
Line range hint
201-201: Approve text change and suggest consistency.The change from "LOGOUT" to "Log Out" in the mobile menu improves the text formatting and aligns with common UI practices. This enhancement contributes to a better user experience, which is in line with the PR objectives.
For consistency, consider applying the same casing style to other buttons in the application, such as the "LOGIN" button. This will ensure a uniform look across the UI.
Let's check for other instances of "LOGIN" or "LOGOUT" in the codebase:
#!/bin/bash # Search for LOGIN or LOGOUT in all files rg -i '\b(login|logout)\b' -nThis will help us identify any other occurrences that might need to be updated for consistency.
73-76: Verify the intention behind changing the z-index.The z-index of the navbar has been reduced from 50 to 20. This change might affect the visibility of the navbar in relation to other elements on the page.
Could you please clarify the reasoning behind this change? Are there any specific UI elements that should now appear above the navbar?
To help assess the impact, let's check for other z-index values in the codebase:
This will help us understand if there are any potential conflicts with other elements' z-index values.
README.md (3)
151-154: LGTM: Contributors section updated appropriately.The changes to the contributors section have been implemented correctly. The updates maintain the existing structure and formatting of the contributors table while adding new contributors and updating existing entries.
Also applies to: 164-165, 181-184, 194-200, 208-209, 232-235, 239-242, 252-253
164-165: Note: Linter warnings about hard tabs can be ignored.The static analysis tool (Markdownlint) has flagged the use of hard tabs in the HTML tags of the contributors table. However, these warnings can be safely ignored for the following reasons:
- The hard tabs are used in HTML tags within a Markdown file, which doesn't affect the rendered output.
- Changing the indentation of these HTML tags is unlikely to improve readability or functionality.
- Modifying these lines might interfere with any automated processes that manage the contributors table.
No action is required for these linter warnings.
Also applies to: 208-209, 252-253, 296-297
🧰 Tools
🪛 Markdownlint
164-164: Column: 1
Hard tabs(MD010, no-hard-tabs)
165-165: Column: 1
Hard tabs(MD010, no-hard-tabs)
Line range hint
1-340: LGTM: README.md updated successfully.The changes to the README.md file have been reviewed and are approved. The updates primarily focus on the contributors section, adding new contributors and updating existing entries. These changes:
- Maintain the existing structure and formatting of the document.
- Properly recognize and credit project contributors.
- Do not introduce any issues or inconsistencies in the documentation.
The pull request can be merged as is.
🧰 Tools
🪛 Markdownlint
208-208: Column: 1
Hard tabs(MD010, no-hard-tabs)
209-209: Column: 1
Hard tabs(MD010, no-hard-tabs)
| // useEffect to add/remove class to body for disabling scroll when modal is visible | ||
| useEffect(() => { | ||
| if (isVisible) { | ||
| document.body.style.overflow = 'hidden'; | ||
| } else { | ||
| document.body.style.overflow = 'auto'; | ||
| } | ||
| return () => { | ||
| document.body.style.overflow = 'auto'; | ||
| }; | ||
| }, [isVisible]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using CSS classes for managing body overflow.
While the current implementation works, directly manipulating the DOM can sometimes lead to unexpected behavior, especially in more complex applications. Consider using CSS classes to manage the body overflow instead.
Here's an alternative approach using CSS classes:
- Add a class to your global CSS file:
.modal-open {
overflow: hidden;
}- Modify the useEffect hook as follows:
useEffect(() => {
if (isVisible) {
- document.body.style.overflow = 'hidden';
+ document.body.classList.add('modal-open');
} else {
- document.body.style.overflow = 'auto';
+ document.body.classList.remove('modal-open');
}
return () => {
- document.body.style.overflow = 'auto';
+ document.body.classList.remove('modal-open');
};
}, [isVisible]);This approach is more declarative and easier to maintain, especially if you need to add more styles to the body when the modal is open.
|
@dev129 increase the size |
|
Very well, but you tell me all the changes at once, it becomes easier for me to maintain. Are you are that is the only change ? |
|
@dev129 you need to just commit with your branch |
|
This PR has been automatically closed due to inactivity from the owner for 3 days. |
PR Title: Add Offer Modal with Highlighted CTA Button (#196)
Description:
This PR addresses issue #196 by implementing an exciting offer modal to encourage users to explore the menu and discover more about the café. The modal introduces a clear and engaging call-to-action (CTA) with the following key features:
/menupage.Changes Included:
Impact:
This feature not only drives users to check the menu but also encourages them to engage further with the website, potentially increasing traffic and conversions for promotional offers.
Please review the changes and provide feedback if necessary!
Closes #196 .
Summary by CodeRabbit